fix(agent): invalidate system prompt cache for global/builtin skills#845
fix(agent): invalidate system prompt cache for global/builtin skills#845pikaxinge wants to merge 3 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a cache invalidation bug where the system prompt cache was not being invalidated when global or builtin skills were modified. Previously, only workspace skills were being tracked for cache invalidation, causing stale cached prompts to be served when skills from other locations changed.
Changes:
- Added
SkillRoots()method to expose all unique skill root directories in priority order (workspace > global > builtin) - Updated cache baseline tracking and invalidation logic to monitor all skill roots instead of just workspace skills
- Added comprehensive regression tests for global and builtin skill file changes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/skills/loader.go | Added SkillRoots() method to return deduplicated skill roots in priority order, serving as single source of truth |
| pkg/agent/context.go | Updated skillRoots(), buildCacheBaseline(), and sourceFilesChangedLocked() to track and check all skill roots for changes |
| pkg/agent/context_cache_test.go | Added TestGlobalSkillFileContentChange and TestBuiltinSkillFileContentChange to verify cache invalidation works for all skill sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Clean fix for a real cache staleness bug. The issue is straightforward: BuildSkillsSummary() reads from workspace, global (~/.picoclaw/skills), and builtin (cwd/skills) skill roots, but cache invalidation only tracked the workspace skills directory. Editing a global or builtin skill would not invalidate the cached system prompt, serving stale content until a restart.
The fix:
SkillRoots()inloader.goexposes all skill root directories with deduplicationskillRoots()incontext.godelegates to the loader (with fallback to workspace/skills if no loader)buildCacheBaseline()andsourceFilesChangedLocked()now iterate all roots instead of just the workspace
Code quality observations:
- The deduplication in
SkillRoots()viafilepath.Clean+ seen-set handles the case where workspace == builtin (when running from the workspace directory). Good. - The empty-string guard (
strings.TrimSpace(root) == "") prevents walking from the filesystem root. - Tests cover both global and builtin skill modifications with mtime manipulation to force cache detection.
One potential issue (non-blocking): The builtin test (TestBuiltinSkillFileContentChange) changes os.Chdir() to set the builtin root, and uses t.Cleanup to restore. This is fine for the test, but os.Chdir affects the entire process, which could interfere with parallel tests. Consider using t.Setenv("PICOCLAW_BUILTIN_SKILLS", builtinRoot) if the loader supports it, or adding t.Parallel() exclusion.
LGTM.
pikaxinge
force-pushed
the
fix/skills-cache-invalidation-skill-roots
branch
from
d73994c to
ae7d36d
Compare
|
Updated based on review follow-up:
Verification:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @lxowalle and @yinwm, could you please take a look at this PR when you have time? This PR fixes stale system-prompt cache invalidation for skills. What changed:
Verification:
Thanks for your time and feedback. |
Summary
SkillRoots()inSkillsLoaderas the single source of truth for tracked skill rootsRoot Cause
BuildSkillsSummary()reads from workspace/global/builtin skill roots, but cache invalidation only tracked workspace files andworkspace/skills. This could keep serving stale cached system prompts when global or builtin skills changed.Changes
pkg/skills/loader.goSkillRoots()to expose unique effective skill roots in priority orderpkg/agent/context.goexistedAtCache+maxMtime)sourceFilesChangedLocked()pkg/agent/context_cache_test.goTestGlobalSkillFileContentChangeTestBuiltinSkillFileContentChangeTest Plan
go test ./pkg/agent -run 'Test(GlobalSkillFileContentChange|BuiltinSkillFileContentChange)'go test ./pkg/agent ./pkg/skillsgo test ./...